Repository navigation
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
9c57c3e to
bb89d46
Compare
This comment has been minimized.
This comment has been minimized.
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One regression needs correction before merge: this patch reverses the FI backup-discovery fix from #2344.
bb89d46 to
c5319a4
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
No actionable findings in the five-file change. Source inspection supports preservation of ranking keys and stable ties, append-only chooser rows, full-candidate Space checks and the 50-row truncation threshold. The public description and feature commit text were inspected; the description has no attached images.
Reviewed head: c5319a4. Target base: cbc71c8. PR merge base: 67f6229.
Source-only: no tests, builds, app launches or PR code execution. A read-only hosted-check snapshot reports CI required successful; Windows native validation and browser flake checks were skipped. Reported browser timing improvements were not independently reproduced. Native/human acceptance, rendered accessibility and runtime focus/retry behavior remain unverified; this comment is not approval or merge authorization.
In a 9,000-member channel the open mention menu spent one long frame on ranking work when the roster finished loading: - The installed rows sorted every choice and kept 50. `rankMentions` now takes a limit and keeps the first rows of the same order in one pass. - The Space check folded every name to find names that continue an empty query. No name continues an empty query, so `continuingNames` returns early. - Outside choices are ranked only when a row can still be added, not on every render with 50 rows installed. Sampled browser measurement (profile-roster-scale, local Chromium, 3 runs each, main 15febc1 vs this change): longest sampled frame 116.7 ms -> 50 ms, sampled frames over 50 ms 1-2 -> 0, jank 118-134 ms -> 51-68 ms. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
The chooser's 50-row limit was a literal in ranking, appending and the inline completion budget. One MENTION_ROWS constant now owns it, and a hook test pins that more members than that show exactly that many rows. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
c5319a4 to
2af321a
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, with independent Rocket, Mantis and Groot coverage.
No new actionable findings in this rebased five-file change. The ranking limit preserves the full sort’s prefix and append-only chooser order; Space resolution still checks the uncapped candidate set. The updated hook and its inline/picker consumers retain live selection checks and error/retry handling. Removing the obsolete truncation helper is consistent with the updated base. The public PR description and feature commit messages were also inspected; there are no attached images.
Reviewed head 2af321a8332f18ab1ce863549fc3767016b13e8c against base 49bdc82b9e29c876e0b709b0c3de263b75e5db3b, with comparison to previously reviewed c5319a4aed541c3d550b255a7dcf582cc9d2c8c1. This follow-up covers the feature’s changed integration, not an independent review of all intervening main-branch changes.
Source-only: no tests, builds, app launches or profiling were performed. Rendered focus/accessibility, human acceptance and the reported frame timings remain unverified; the timing samples identify a different baseline (15febc12e). A one-time hosted-check snapshot still had browser, JavaScript, Rust and measurement jobs running; Windows validation was skipped. This COMMENT is not approval or merge authorization.
🤖
Summary
@opened a mention menu that froze the app for about 117 ms when the member list finished loading. Most of that time was spent ranking names the menu never shows.profile-roster-scale(local Chromium, 3 runs each, on main 15febc1)Frame times vary with machine load. An earlier version of this PR, on main fe6eaa2, measured 150–167 ms on main; #2328 has since removed the
truncatedflag, which was part of that cost.Details
rankMentionsnow takes alimitand keeps the first rows of the same order in one pass (a bounded insertion, about one comparison per name). The menu always passesMENTION_ROWS(50), the most rows it shows.continuingNamesnow returns early for one.exactNamealready returned nothing for an empty query.MENTION_ROWS, shared by ranking, appending and the inline completion budget. A hook test checks that 51 members show exactly 50 rows; it fails with the ranking limit removed.The ranking test checks that a limited ranking equals the first rows of the full ranking for 240 mixed choices, 6 queries and several limits, and that an empty query has no continuing or exact name.
Not in this PR
The remaining ~50 ms sampled frame is not ranking. It is name labels (~40 ms), building the candidate list (~30 ms) and signature checks for the last profile page (~25 ms), from a profiled build on the earlier base. A non-empty query also still rebuilds the name index whenever the candidate list changes.
🤖 Generated with Claude Code